Skip to content

Possible naming improvements via codegen changes#1043

Draft
SteveSandersonMS wants to merge 7 commits intomainfrom
stevesa/rpc-naming-improvments
Draft

Possible naming improvements via codegen changes#1043
SteveSandersonMS wants to merge 7 commits intomainfrom
stevesa/rpc-naming-improvments

Conversation

@SteveSandersonMS
Copy link
Copy Markdown
Contributor

@SteveSandersonMS SteveSandersonMS commented Apr 8, 2026

Draft of possible changes to RPC naming convention changes to fix #1035

This PR improves generated public type names across the SDKs by moving naming decisions into the shared schema/codegen pipeline instead of relying on language-specific quirks.

The goal is to make generated names:

  • shorter when the old names were just path concatenation noise
  • more specific when the old names were too generic to be useful
  • more consistent across TypeScript, C#, Go, and Python
  • more stable, because naming now follows a common set of rules plus explicit schema-local overrides where the API really wants a specific public name

Main naming rules

The naming is now mostly mechanical rather than hand-curated:

  1. Derive names from the RPC method / event identity first

    • Example: account.getQuota becomes an AccountQuota root instead of AccountGetQuotaResult.
  2. Drop generic namespace and filler words

    • Strip prefixes like session.
    • Strip terminal filler verbs like get, read, and list when they are only structural
  3. Singularize list entities

    • Example: models.list uses Model for nested item types and ModelList for the top-level result
  4. Keep top-level wrapper words only when they are actually useful

    • Request / Result remain for top-level params/results
    • nested helper types stop inheriting repeated ...Request...Result...Value...Item noise
  5. Drop purely structural wrapper property names in nested helpers

    • Words like data, result, request, response, kind, value, etc. are removed when they are just containers, not meaningful domain terms
  6. Normalize overlapping words

    • Avoid awkward duplication like requested + request
  7. Use explicit schema-local names for true API surface decisions

    • When the API really wants a specific public name, that name is declared in the schema instead of in generator-side override maps

Before / after examples

Previously too long

Before After
AccountGetQuotaResultQuotaSnapshotsValue AccountQuotaSnapshot
SessionPermissionsHandlePendingPermissionRequestResult PermissionRequestResult
SessionToolsHandlePendingToolCallResult HandleToolCallResult
SessionUiElicitationRequestRequestedSchema UiElicitationSchema
SessionUiHandlePendingElicitationRequestResult UiElicitationResponse
SessionUiHandlePendingElicitationResult UiElicitationResult
ToolExecutionCompleteDataResultContentsItemResourceLinkIconsItemTheme ToolExecutionCompleteContentResourceLinkIconTheme
UserMessageDataAttachmentsItemSelectionSelectionEnd UserMessageAttachmentSelectionDetailsEnd

Previously too short / too generic

Before After
Entry SessionFsReaddirWithTypesEntry / SessionFSReaddirWithTypesEntry
End UserMessageAttachmentSelectionDetailsEnd
Mode SessionMode
SessionModeGetResultMode SessionMode

A few especially notable cases

  • The worst "path explosion" example was the old icon theme helper name:

    • ToolExecutionCompleteDataResultContentsItemResourceLinkIconsItemTheme
    • now: ToolExecutionCompleteContentResourceLinkIconTheme
  • The worst "too generic to understand in isolation" examples were names like:

    • Entry
    • End
    • Mode

    Those now carry enough domain context to be understandable in generated SDKs without needing to inspect the surrounding method/property path.

API-specific naming that is now explicit

A few names are intentionally chosen as API surface names rather than just synthesized mechanically, for example:

  • UiElicitationResponse
  • UiElicitationResult
  • HandlePendingElicitationRequest
  • PermissionDecision
  • PermissionDecisionRequest
  • PermissionRequestResult
  • ModeGetResult
  • ModeSetResult
  • HandleToolCallResult

That keeps the generator logic generic while still letting the schema declare clear public names where needed.

TypeScript note

TypeScript still avoids exploding the surface area with every synthesized helper as a named export. Explicit public names are preserved where intended, while synthetic helper naming is mainly used to improve the non-TS SDKs and shared generation consistency.

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated by SDK Consistency Review Agent for issue #1043

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated by SDK Consistency Review Agent for issue #1043

@SteveSandersonMS
Copy link
Copy Markdown
Contributor Author

Addressed the review findings in add04df. Specifically: (1) TS now deduplicates repeated named export blocks from per-method json-schema-to-typescript compiles, which removes the duplicate SessionMode and duplicate UiElicitationResponse declarations; and (2) the Go/Python quicktype-based generators now collapse placeholder *Class duplicates onto the explicit titled type when the structures are identical, so HandlePendingElicitationRequest.result now uses UIElicitationResponse instead of ResultClass.

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated by SDK Consistency Review Agent for issue #1043



class HostType(Enum):
class ContextChangedHostType(Enum):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming inconsistency: This enum is named ContextChangedHostType here but StartContextHostType in Go and .NET. Consider adding a title/titleSuggestion to this schema node so quicktype produces StartContextHostType to match.



class Operation(Enum):
class ChangedOperation(Enum):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming inconsistency: Python collapses both the plan.changed and workspace.fileChanged operation enums into a single ChangedOperation, while Go and .NET produce two separate enums PlanChangedOperation and WorkspaceFileChangedOperation. If they should be distinct, schema-level title/titleSuggestion overrides on each individual enum schema would resolve this.



class PermissionRequestKind(Enum):
class Kind(Enum):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming inconsistency: Kind is too generic and lacks context. Go names this PermissionRequestKind (renamed from PermissionRequestedDataPermissionRequestKind in this PR). Adding a titleSuggestion: "PermissionRequestKind" to this schema node would align the Python output with Go.

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated by SDK Consistency Review Agent for issue #1043

ExtensionsLoadedExtensionStatusStarting ExtensionsLoadedExtensionStatus = "starting"
)

// Type aliases for convenience.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Go compilation break: TYPE_ALIASES and CONST_ALIASES reference old type names

The convenience type/const aliases below this line still reference the old generated type names that no longer exist in this file after the renames. For example:

// TYPE_ALIASES in scripts/codegen/go.ts — still points to OLD names:
PermissionRequest        = PermissionRequestedDataPermissionRequest        // ❌ undefined; PermissionRequest is now a struct directly
PermissionRequestKind    = PermissionRequestedDataPermissionRequestKind    // ❌ redeclared (PermissionRequestKind is now a string type at line ~2227)
PermissionRequestCommand = PermissionRequestedDataPermissionRequestCommandsItem  // ❌ undefined
PossibleURL              = PermissionRequestedDataPermissionRequestPossibleUrlsItem // ❌ undefined
Attachment               = UserMessageDataAttachmentsItem                  // ❌ undefined
AttachmentType           = UserMessageDataAttachmentsItemType              // ❌ undefined

And the CONST_ALIASES block similarly references UserMessageDataAttachmentsItemTypeFile, PermissionRequestedDataPermissionRequestKindShell, etc. — all of which have been renamed.

The TYPE_ALIASES and CONST_ALIASES maps in scripts/codegen/go.ts (around line 800) were not updated as part of this PR. They need to be updated to reflect the new generated names, e.g.:

// Suggested fixes:
PossibleURL      = PermissionRequestShellPossibleUrl
Attachment       = UserMessageAttachment
AttachmentType   = UserMessageAttachmentType

// Const aliases:
AttachmentTypeFile = UserMessageAttachmentTypeFile
// etc.

Types like PermissionRequest and PermissionRequestKind are now direct struct/type declarations, so their alias entries can simply be removed (or the alias should map to the new canonical name if needed for backward compat).

type SessionPermissionsHandlePendingPermissionRequestParams struct {
RequestID string `json:"requestId"`
Result SessionPermissionsHandlePendingPermissionRequestParamsResult `json:"result"`
type PermissionDecisionRequest struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Go compilation break: non-generated SDK code references old type names

This PR renames SessionPermissionsHandlePendingPermissionRequestParamsPermissionDecisionRequest (and similarly for other types), but the non-generated Go SDK code in go/session.go and go/types.go still references the old names. A full list of ~43 identifiers from the rpc package that are missing includes:

Old name (used in non-generated code) New name (in this PR)
rpc.SessionCommandsHandlePendingCommandParams rpc.CommandsHandlePendingCommandRequest
rpc.SessionUIHandlePendingElicitationParams rpc.HandlePendingElicitationRequest
rpc.SessionUIHandlePendingElicitationParamsResult rpc.UIElicitationResponse
rpc.SessionPermissionsHandlePendingPermissionRequestParams rpc.PermissionDecisionRequest
rpc.ActionAccept / rpc.ActionCancel rpc.UIElicitationActionAccept / rpc.UIElicitationActionCancel
rpc.PropertyTypeBoolean / rpc.PropertyTypeString rpc.UIElicitationSchemaPropertyNumberTypeBoolean / ...String
rpc.SessionUIElicitationParams (structure changed)
rpc.ConventionsPosix / rpc.ConventionsWindows rpc.SessionFSSetProviderConventionsPosix / ...Windows
rpc.EntryTypeDirectory / rpc.EntryTypeFile rpc.SessionFSReaddirWithTypesEntryTypeDirectory / ...File
rpc.ModeInteractive / rpc.ModePlan rpc.SessionModeInteractive / rpc.SessionModePlan
rpc.LevelError etc. rpc.LogLevelError etc.
All rpc.SessionFS*Params rpc.SessionFS*Request

go/session.go, go/types.go, and other non-generated files will need to be updated to use the new names. This is expected to be a significant follow-up change to make the Go SDK compile.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

/// <summary>RPC data type for SessionModeSet operations.</summary>
public class SessionModeSetResult
/// <summary>RPC data type for ModeSet operations.</summary>
public class ModeSetResult
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed (rather than void) because the set operation might fail?

Copy link
Copy Markdown
Contributor Author

@SteveSandersonMS SteveSandersonMS Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's strictly needed but I can certainly imagine some future scenario where you ask for mode X but we decide that's not valid right now and return some other mode that we fell back on.

In any case we should make sure we can have void-returning RPC methods in case we want that. I've updated the codegenerator so that it does support that for all languages.

Note that the Go variant of this is weird. Unlike the other languages, changing from void to non-void would be a breaking change because we'd go from:

func (a *ModeApi) Set(ctx context.Context, params *ModeSetRequest) error { ... }

to:

func (a *ModeApi) Set(ctx context.Context, params *ModeSetRequest) (*SessionMode, error) { ... }

... and so caller code that operates on error (i.e., all well-written code) would have to be updated.

To handle this I'm updating the Go codegen specifically so that it doesn't use error on its own as a result type and always produces synthetic/empty structs so in this case it returns (*SessionModeSetResult, error) even though SessionModeSetResult is an empty struct.

According to Copilot this is not completely weird in Go - gRPC/protobuf does the same thing in its own codegenerator.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this still constrains us in the future. We cannot later change mode.set to return SessionMode. We would have to change it to return a wrapper object, SessionModeSetResult, otherwise it would be a breaking change for Go.

/// <summary>RPC data type for SessionPlanUpdate operations.</summary>
public class SessionPlanUpdateResult
/// <summary>RPC data type for PlanUpdate operations.</summary>
public class PlanUpdateResult
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these namings sound a little odd to my ears, e.g. PlanUpdateResult vs UpdatePlanResult. But not a big deal.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reversing the noun-verb order probably requires us to set names at each point in the schema on the runtime side. We can consider doing that when we do a full API review but would be most practical to keep that separate from the current goal to improve mechanical naming.

[Experimental(Diagnostics.Experimental)]
public class SessionAgentListResult
public class AgentList
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For C#, we could consider nesting types, having the request/response types for an RPC API be nested within the type that API is defined on, e.g. have AgentList be nested inside of AgentApi. That way, we minimize chances of conflict by giving them a namespace, e.g. when we add an API for getting a list of all subagents, if we were to just call them "agent", we'd have a conflict, but not if that listing method and the one for defined custom agents were on different types.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that might be better for C#, but there'll be limited value in avoiding conflicts for C# if the same types still conflict in other languages. Definitely open to doing something like this when we do a full API review if we conclude there's a pattern that's going to work across languages.

[Experimental(Diagnostics.Experimental)]
public class SessionAgentGetCurrentResult
public class AgentCurrent
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another case where I'm wondering if the wrapper is adding any value. For cases where we already need to return multiple pieces of data, of course, and for cases where we think we will, makes sense... but in a case like this, I struggle to think what else we might need to return, in which case this needn't be in the public API surface area and the relevant method can just return an AgentCurrentAgent (which itself is a questionable name)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but would it be OK to limit this PR to changes that affect the codegen machinery rather than being an API review in general? For any changes that force us to update the codegenerator I'll definitely try to handle them here, but for ones that are purely schema updates on the runtime side it would be good to handle them separately as there may be a pretty long tail.

@@ -711,13 +747,29 @@ internal class SessionAgentDeselectRequest
public string SessionId { get; set; } = string.Empty;
}

/// <summary>RPC data type for SessionAgentReload operations.</summary>
/// <summary>RPC data type for AgentReloadAgent operations.</summary>
public class AgentReloadAgent
Copy link
Copy Markdown
Collaborator

@stephentoub stephentoub Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to have multiple types that are identical other than in name, e.g. select agent, current agent, reload agent... can we do something to dedup those in the SDK? That would then also help with naming, as we wouldn't need to have multiple names for the exact same thing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1062 is adding $ref support. And https://github.com/github/copilot-agent-runtime/pull/6059 takes advantage of that to avoid a bazillion copies of the same data type. If we like this, we can use it to dedup in the existing schema.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great, thanks!

[Experimental(Diagnostics.Experimental)]
public class SessionMcpDisableResult
public class McpDisableResult
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we plan to add more to a type like this, can we model that in the SDK by just having the method return void / Task instead of an empty dummy object?

Copy link
Copy Markdown
Contributor Author

@SteveSandersonMS SteveSandersonMS Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make that change but it doesn't give us total freedom in the future. If we later want to make it non-void, we will be restricted to making it return McpDisableResult (and not a primitive or any other type name) otherwise it will be a breaking change for Go.

{
/// <summary>Entry name.</summary>
[JsonPropertyName("name")]
public string Name { get; set; } = string.Empty;

/// <summary>Entry type.</summary>
[JsonPropertyName("type")]
public EntryType Type { get; set; }
public SessionFsReaddirWithTypesEntryType Type { get; set; }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For .NET, I expect we'd want "Readdir" to be "ReadDir" or "ReadDirectory".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that would be more conventional for .NET but you could equally say that for Node it should be readdir to match the fs APIs. Let's later consider whether we want to add annotations for per-language names. Trying to make truly optimal APIs across all languages might be counterproductive.

Plugin,
/// <summary>The <c>builtin</c> variant.</summary>
[JsonStringEnumMemberName("builtin")]
Builtin,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builtin or BuiltIn ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's come back to that when we do an API review

@edburns
Copy link
Copy Markdown
Contributor

edburns commented Apr 10, 2026

Full disclosure 01: using my human brain Java subject matter expertise, Opus 4.6, and existing context, I arrived at these comments, in light of the comments Stephen posted to Slack.

Full disclosure 02: The current agentic workflow derives the Java SDK directly from the post-code-generated parts of the reference implementation. I have an outstanding work item ( https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2751554 ) to make it so the Java SDK also uses the Zod schema, in addition to the agentic workflow to reduce toil in keeping the Java SDK in synch with the .NET and TypeScript reference implementations (See agentic workflow). Therefore, my comments are to be taken in light of what I would like to see before I undertake the work to implement dd-2751554-do-code-gen-for-java.

Disclosures aside, I stand by the following comments.

1. Reinforce @stephentoub 's point on empty types — with a Java-specific angle

+1 on eliminating empty result types. From the Java SDK perspective, the 13 empty types (PlanUpdateResult, AgentDeselect, SkillsEnableResult, McpReload, etc.) present an awkward ergonomics issue. Java doesn't have Task vs Task<T> — we use CompletableFuture<Void> for void results. Generating an empty record like public record McpReload() {} would feel unnatural to Java developers and force consumers to handle a meaningless return type. If the schema expressed these as null/void results, each language generator could map to its natural void representation (Task in C#, CompletableFuture<Void> in Java, None in Python, empty struct or no return in Go).

Question: Would it make sense for empty result schemas to be represented as null in the API schema rather than { "type": "object", "properties": {} }? That way generators could choose void/Void without needing per-language special-casing. Happy to discuss if there's a reason the current shape is preferred.

2. Reinforce the duplicate types point — with specific examples

+1 on consolidating identical structures. The Agent group stood out to me: Agent, AgentCurrentAgent, AgentSelectAgent, and AgentReloadAgent are structurally identical (all have name, displayName, description). Similarly, ModelCurrent and ModelSwitchToResult both wrap a single modelId: string?. And four types (UiElicitationResult, PermissionRequestResult, HandleToolCallResult, CommandsHandlePendingCommandResult) are all { success: boolean }.

In Java, each of these becomes a separate record class. The naming machinery in PR 5908 does a nice job producing clean names for them, but I wonder if we could go a step further: would it be feasible to define a shared Agent schema (with $ref) and have agent.select, agent.current, and agent.reload all reference it? I noticed that hoistTitledSchemas() already deduplicates schemas that share the same title — so maybe it's just a matter of giving these inline definitions a shared title? I might be missing a reason they need to be separate.

3. Namespacing: Java has a different approach than C# nested types

On the namespacing suggestion: For context on how this lands in Java — our natural namespacing mechanism is packages, not nested classes. Where C# would nest Agent.SelectResult inside a class Agent, Java would typically use com.github.copilot.sdk.rpc.agents.SelectResult. This tends to work well in practice because:

  • It avoids the Agent.Agent pattern (outer class and inner class sharing a name, which is a Java code smell).
  • It aligns with how Jackson deserialization resolves types.
  • It enables per-package package-info.java for group documentation.

One thing I'd love to explore: package-based namespacing would benefit from the schema carrying some grouping information so generators know which package/namespace a type belongs to. Today, the schema titles are flat strings (AgentSelectResult, McpConfigServerLocal). I'm wondering whether it might be worth considering an optional group or namespace annotation alongside title — that way each language generator could map to its natural grouping mechanism (packages in Java, nested types in C#, module prefixes in Python) without parsing the group from the type name string. This may be out of scope for this PR, but wanted to plant the seed.

4. Java-specific observation: the MCP filter mapping type explosion

From the Java SDK perspective, the MCP config types are a notable hotspot. There are 18 FilterMapping variant types across 6 contexts (server local, server http, add local, add http, update local, update http) — structurally identical in groups of 3. In Java, each would become a separate sealed interface + record hierarchy, with identical Jackson @JsonTypeInfo/@JsonSubTypes annotations multiplied 6×.

This hasn't been an issue in practice because the Java SDK hasn't implemented MCP config types yet (it was easy to skip when hand-writing). With codegen, they'd all materialize automatically. I'm curious — is there a reason these can't share a single McpConfigFilterMapping definition with $ref? There may be a forward-compatibility reason I'm not seeing for keeping them separate.

5. Stability guarantee + Java sealed types interaction

The stability guarantee from PR 5908 (new schemas never rename existing types) is very welcome. One subtlety for Java: with sealed interfaces/classes, adding a new event type requires updating the permits clause on the base type. This means the generated base type changes on every new event — which is fine since it's generated. But it also means consumers who do exhaustive switch on event types will get compile warnings when new events are added. This is actually desirable behavior (it's the point of sealed types), but it's worth documenting as an expected contract: adding a new event type in the runtime is a source-compatible but not pattern-exhaustiveness-compatible change.

6. Single-property wrappers: a different perspective from the Java side

On single-property wrapper types: I wanted to offer a slightly different perspective from the Java side. Stephen's suggestion to return the raw value directly (e.g., string instead of ShellExecResult { processId: string }) makes a lot of sense for .NET ergonomics. In Java, there's a tension: CompletableFuture<String> is less self-documenting than CompletableFuture<ShellExecResult>, and unwrapping removes the ability to add fields later without a breaking change.

One possible approach: Keep the wrapper types in the schema (for extensibility), but mark single-property types with a schema annotation (e.g., "unwrappable": true). Generators that prefer unwrapping (C#) could do so; generators that prefer wrapper types (Java, for future-proofing) could ignore it. This would avoid baking an unwrapping decision into the shared schema that all languages must follow. That said, I recognize this adds complexity to the schema — I'm open to simpler alternatives if others have ideas.

Comment on lines 66 to 67
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this isn't naming-related, but lots of APIs use double when they would be better represented as int, TimeSpan, etc. It would be nice if we could support a richer set of numeric types in generated code.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the C# generator to use long for integer, and I've merged an update into copilot-agent-runtime that fixes a bunch of properties typed as number to instead be typed as integer. If we want to take this a step further, we could update the schema in runtime to use min/max in the schema, and then in the generators use those ranges to further constrain the type, e.g. if max is <= int.MaxValue, use int instead of long.

@@ -2297,17 +2513,17 @@ internal UiApi(JsonRpc rpc, string sessionId)
}

/// <summary>Calls "session.ui.elicitation".</summary>
public async Task<SessionUiElicitationResult> ElicitationAsync(string message, SessionUiElicitationRequestRequestedSchema requestedSchema, CancellationToken cancellationToken = default)
public async Task<UiElicitationResponse> ElicitationAsync(string message, UiElicitationSchema requestedSchema, CancellationToken cancellationToken = default)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ElicitAsync might be a better name for this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to keep the UI prefix for disambiguation with other kinds of elicitation

Comment on lines 814 to 819
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this is a wrapper type that probably doesn't have to exist (could we just use List<Skill> directly)?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see similar patterns elsewhere (like McpList)

/// <summary>RPC data type for SessionUiElicitation operations.</summary>
public class SessionUiElicitationResult
/// <summary>The elicitation response (accept with form values, decline, or cancel).</summary>
public class UiElicitationResponse
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a nit, but I think UI feels more natural than Ui.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - seems like one for the API review.

}

/// <summary>Token usage metrics for this model.</summary>
public class UsageMetricsModelMetricUsage
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these names still seem a bit unwieldy (though for cases like this I anticipate we could just use .withTypeName and override it).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - seems like one for the API review.

Comment on lines +1436 to +1445
public class UsageMetricsModelMetric
{
/// <summary>Request count and cost metrics for this model.</summary>
[JsonPropertyName("requests")]
public UsageMetricsModelMetricRequests Requests { get => field ??= new(); set; }

/// <summary>Token usage metrics for this model.</summary>
[JsonPropertyName("usage")]
public UsageMetricsModelMetricUsage Usage { get => field ??= new(); set; }
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if some of these would make sense as nested types.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially, but let's separate that out into future runtime-side schema changes.

@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/rpc-naming-improvments branch from e6d40bf to 13195cd Compare April 13, 2026 10:40
Rebased branch onto latest origin/main, resolved codegen script
conflicts (csharp.ts emittedRpcClassSchemas Map merge, Session.cs
and Types.cs type name updates), and regenerated all 4 languages
from latest runtime schemas.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/rpc-naming-improvments branch from 13195cd to 216407b Compare April 13, 2026 10:54
@github-actions

This comment has been minimized.

SteveSandersonMS and others added 2 commits April 13, 2026 12:04
Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
…urn void

Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
@SteveSandersonMS
Copy link
Copy Markdown
Contributor Author

About empty result types: there were about 15 of these, and I've removed them all.

Well, the reality is a bit more complex: for Go we need to retain them for forward compat, which in turn means we also need to retain the name generator output from the runtime so we wouldn't break them name in the future if we changed from void to nonvoid.

I've updated things on both sides so that Go is able to produce empty result structs with the desired names, and the other languages use their own versions of void.

Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated by SDK Consistency Review Agent for issue #1043 · ● 1.8M

@SteveSandersonMS
Copy link
Copy Markdown
Contributor Author

SteveSandersonMS commented Apr 13, 2026

  1. Reinforce @stephentoub 's point on empty types — with a Java-specific angle
    +1 on eliminating empty result types.

This is done, though it's not a perfect solution given that in Go it creates an extra forward-compat concern. The middle ground we're in now is:

  • For Go, it continues to produce empty result types so the return value can be (*SomeType, error)
  • For other languages it now uses their version of void/async-void

In the future, if we want to change these methods to be nonvoid, we will be constrained to give them the type SomeType and not anything else (not a primitive, not some other name) for compatibility with existing Go code.

  1. Reinforce the duplicate types point — with specific examples

Valid but we must do this through runtime schema changes (e.g., using $ref). We can't collapse types just because they are structurally equivalent as that would be breaking if they stop being equivalent later. So likely we should keep that out of scope until we add $ref in @stephentoub's PR.

  1. Namespacing: Java has a different approach than C# nested types

This PR is already quite hard to land so would be good to get in what we have before we think about systems for nesting types. Given the large differences between languages (e.g., how TypeScript doesn't have any global namespace) it might or might not be productive to try to have a unified model here.

  1. Java-specific observation: the MCP filter mapping type explosion

We can change whatever we think is best on the runtime schema side. For this PR I'm trying to focus on the machinery that generates the JSON schema and then renders that out into per-language code.

  1. Stability guarantee + Java sealed types interaction
    adding a new event type in the runtime is a source-compatible but not pattern-exhaustiveness-compatible change.

Understood, thanks.

  1. Single-property wrappers: a different perspective from the Java side

With the example given (CompletableFuture<String> vs CompletableFuture<ShellExecResult>), it's no different from in .NET (Task<string> vs Task<ShellExecResult>). So I'm not sure there's any Java-vs-.NET reason to vary the representation. We can't say that the ability to add more properties in the future is a reason to have some but not all languages use wrappers - either they must all do it, or none of them can, since changing a primitive return to an object return would be breaking.

The way the machinery works now is that the runtime schema decides whether a given method's return value is a raw primitive or is wrapped in some object. The person authoring that schema has to decide on the likelihood of needing to add extra properties in the future.

I think our options are:

  • Risk averse - say we always think more might be added, and hence require every RPC method to return a wrapper *Result type (even things that would otherwise be void). That's what we had before all this work but @stephentoub was recommending not to do that.
  • Let the runtime schema decide - we have a case-by-case decision about how we want to represent the RPC result and whether it might be expandable with other properties in the future. The ergonomics are better, at the cost that it's more painful if our guess turns out to be wrong.

Can we decide which of these two we're going for? I've implemented the second option and am happy to stick with that unless there are big concerns.

@stephentoub
Copy link
Copy Markdown
Collaborator

Let the runtime schema decide - we have a case-by-case decision about how we want to represent the RPC result and whether it might be expandable with other properties in the future.

I think this is the right answer. The runtime here is the source-of-truth for the shape of the API, and the developer of that API needs to say whether there's any possibility it could be expanded in the future.

SteveSandersonMS and others added 2 commits April 13, 2026 14:51
Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review

This PR does a thorough job updating type names across all four SDKs (TypeScript, Go, Python, .NET) — the Python files were included too (pagination limited the initial file listing to 30 files but the diff includes python/copilot/generated/rpc.py, session.py, client.py, and test files).

✅ Well-handled language-idiomatic differences

The following differences are appropriate for each language and don't represent inconsistencies:

Difference Go Python .NET Notes
Empty result types XxxResult {} structs generated Not generated (void) Not generated (void) Language idiom
Union wrapper types Explicit structs (ToolsHandlePendingToolCall, UIElicitationFieldValue) Inline TypeA | TypeB object / polymorphism Language idiom
Acronym casing MCP, FS, UI (all-caps) MCP, FS, UI Mcp, Fs, UI (PascalCase) Language convention
Request type visibility Public Public internal (users don't construct) Language design choice

⚠️ One genuine inconsistency found

MCPServerSource (Go/Python) vs DiscoveredMcpServerSource (.NET)

This PR renamed Go and Python's ServerSource enum → MCPServerSource. However, the .NET SDK was already using DiscoveredMcpServerSource for the same enum values (user/workspace/plugin/builtin). The two names now diverge.

Note: this enum is used for both DiscoveredMCPServer.source and MCPServer.source fields, so the Discovered prefix in .NET is arguably a misnomer regardless.

I've left an inline comment on the Go generated file with more details. The fix would be to either:

  • Rename .NET's DiscoveredMcpServerSourceMcpServerSource in the C# codegen (aligning with Go/Python), or
  • Use DiscoveredMCPServerSource in Go/Python to match .NET

Otherwise the naming improvements in this PR look consistent and well-reasoned. 👍

Generated by SDK Consistency Review Agent for issue #1043 · ● 3.5M ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated by SDK Consistency Review Agent for issue #1043 · ● 3.5M

type ServerSource string
//
// Configuration source: user, workspace, plugin, or builtin
type MCPServerSource string
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-SDK naming inconsistency: MCPServerSource vs DiscoveredMcpServerSource

This PR renames the Go and Python type for MCP server config source from ServerSourceMCPServerSource, but the .NET SDK was already using DiscoveredMcpServerSource for the exact same enum (user/workspace/plugin/builtin). The three SDKs now have different public names for the same concept:

SDK Type name
Go MCPServerSource ← new name from this PR
Python MCPServerSource ← new name from this PR
.NET DiscoveredMcpServerSource ← pre-existing, unchanged

This enum is used in both DiscoveredMCPServer.Source and MCPServer.Source, so the Discovered prefix in the .NET name is arguably a misnomer (it's used for active servers too).

Suggestion: Align on one name. The MCPServerSource naming (without Discovered prefix) seems more accurate since the enum is used for both discovered and active servers. That would mean renaming DiscoveredMcpServerSourceMcpServerSource in the .NET codegen.

Alternatively, keeping Discovered prefix and aligning the other SDKs to DiscoveredMCPServerSource would also resolve the inconsistency — whichever name is preferred.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review all codegenerated type names; provide better names in many cases

4 participants